Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement inline override #8543

Merged
merged 7 commits into from
Mar 25, 2020

Conversation

odersky
Copy link
Contributor

@odersky odersky commented Mar 13, 2020

The idea is to support inline override methods that can be both inlined and called dynamically. The old restriction that an inline method can only override concrete methods will go away. The implementation idea is as follows:

  1. A definition

    inline override def f(x: T) = body
    

    is desugared to two methods:

    inline override def f(x: T) = body
    private def f$retainedBody(x: T) = f(x)
    
  2. At erasure, assume the two methods look like this:

    inline override def f(x: T) = body1
    private def f$retainedBody(x: T) = body2
    

    They are then rewritten to

     override def f(x: T) = body2
    

Here, the owner of body2 is changed to f and all references to parameters of f$retainedBody are
changed to references of corresponding parameters in f.

An open question is whether we should go further and apply this scheme also if there is no override present. This could be useful e.g. for being able to inline unapply methods, since unapply methods have to be kept around at runtime as well. A possible syntax for this would be a soft modifier retained (the opposite of erased). I.e

inline retained unapply(x: T) = ...

This would give an inline unapply that persists after erasure, just like the f above.

@smarter
Copy link
Member

smarter commented Mar 14, 2020

since unapply methods have to be kept around at runtime as well.

Calls to unapply are generated by the typer, so when would we need them to be kept at runtime? In fact, #8542 adds support for inline def unapply

@nicolasstucki
Copy link
Contributor

Note that this could also allow us to support the implementation of an abstract method with an inline method

trait Hello:
  def hello: String

object App extends Hello:
  inline def hello: String = "hi"

@odersky
Copy link
Contributor Author

odersky commented Mar 14, 2020

I am not sure whether we need retained inline unapplys and whether they have to be user-level. @nicolasstucki do you have a typical usecase?

When trying to implement this PR, I noted a potential problem. It's in typeclass-derivation2b.scala, lines 52ff:

    inline override def alternative(inline n: Int) <: GenericProduct[_ <: Lst[T]] =
      inline n match {
        case 0 => Cons.GenericCons[T]
        case 1 => Nil.GenericNil

If we translate that with the described scheme, we get:

53 |      inline n match {
   |             ^
   |             cannot reduce inline match with
   |              scrutinee:  {
   |               n
   |             } : (n : Int)
   |              patterns :  case 0
   |                          case 1

The problem is that the retained version of alternative does not have parameters that are constant, so the inline match fails.

I see two feasible ways around this

  • Make inline override methods that have inline parameters not retained, i.e. use the old scheme for them.
    This works, but feels very subtle and implicit.

  • Invent some more explicit scheme for inline methods to simply shadow other methods, but not override them at runtime. One such possibility would be to bring back erased. E.g. we could reformulate alternative like this:

    inline erased override def alternative(inline n: Int) <: GenericProduct[_ <: Lst[T]] = ...

An erased override is not an override at runtime, only at compile time. So no runtime instance of alternative needs to be created.

Opinions?

Drop unused AvoidClashName name kind and name tags
AVIDCLASH and DIRECT
@sjrd
Copy link
Member

sjrd commented Mar 14, 2020

@odersky The problematic example has a very specific property: the overridden method is never actually correct to begin with. It has = ??? as its rhs, which turns out fine because it's never called at runtime. It should be abstract to begin with, and would certainly be anyway if that had been allowed like we suggest.

Moreover, the example clearly shows that it can only work if that method is overridden in the "virtual" dispatch when it is called. It's just that this always happens statically. Again, this highlights that the true overriding semantics is what this example relies on.

What we need is therefore not a way to bypass overriding semantics with an erased at overriding site. Instead we need a way to mark a method as available for a call, but at compile time it must be checked that it is statically overridden at every call site. That would be the meaning of an abstract inline def, from what I can see.

I would therefore like to declare GenericSum.alternative as an abstract inline def, and GenericLst.alternative as a normal concrete inline def (which would not get a runtime bridge because it only implements an abstract inline method; not because itself is marked as a semantically wrong inline erased thing).

@smarter
Copy link
Member

smarter commented Mar 14, 2020

I see two feasible ways around this

I'm not a fan of either way, seems like too much complexity for too little gains. I think it's OK to have stricter requirements for the bodies of inline override def than inline def.

@nicolasstucki
Copy link
Contributor

I am not sure if the inline override def unapply is needed. I will investigate further.

The code for inline override def alternative does not look problematic as one should just change the implementation to

 inline override def alternative(inline n: Int) <: GenericProduct[_ <: Lst[T]] =
     n match { // remove the inline here
        case 0 => Cons.GenericCons[T]
        case 1 => Nil.GenericNil

This means that if the code is not inlined, the match will be retained. If the code is inlined, it will reduce the match.

Alternatively, we could say the inline match does not check the body of the override inline.

Another alternative is to write the code as follows

 inline override def alternative(inline n: Int) <: GenericProduct[_ <: Lst[T]] =
     inline n match
        case 0 => Cons.GenericCons[T]
        case 1 => Nil.GenericNil
        case _ =>
            // non-specialized implementation (not necessarily the same as the inlined code)
            n match
              case 0 => Cons.GenericCons[T]
              case 1 => Nil.GenericNil

@odersky
Copy link
Contributor Author

odersky commented Mar 15, 2020

@sjrd Good analysis. I think abstract inline could be the way to go here. I'll try that next.

@soronpo
Copy link
Contributor

soronpo commented Mar 15, 2020

Will this have any effect over the ability to combine inline def with opaque type?
lampepfl/dotty-feature-requests#82

@sjrd
Copy link
Member

sjrd commented Mar 15, 2020

@soronpo No it won't, neither positively nor negatively.

```
The inlined invocations and the dynamically dispatched invocations give the same results.

2. Inline methods can override or implement normal methods, as the previous example shows. Inline methods can be overridden only by other inline methods.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hum, I don't think we should allow concrete inline methods to be overridden, even by other inline methods. This can break the semantics of polymorphic dispatch, because we might not flag a call to a method of the superclass on an instance that, at runtime, will be of a subclass.

It works with abstract inline methods because attempts at calling those will be flagged if they cannot be resolved to an overriding concrete inline method.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried with the last commit but it breaks scalatest. We have two inline versions of assert; one in Assertions the other in Diagrams. What scalatest does does not look unreasonable. It's a pity that this means we get static behavior which is not backed by runtime behavior. But it does not look easy to find workarounds in existing software.

So I think we will have to revert this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(for reference the definitions in scalatest are https://github.com/scalatest/scalatest/blob/4e1e38e2372ddada9037d442aa7efed1418119d9/scalatest.dotty/src/main/scala/org/scalatest/Assertions.scala#L470 and https://github.com/scalatest/scalatest/blob/4e1e38e2372ddada9037d442aa7efed1418119d9/scalatest.dotty/src/main/scala/org/scalatest/diagrams/Diagrams.scala#L180).

I'm still strongly in favor of disallowing this. As @sjrd said, it just has the wrong semantics. Scalatest will have to be slightly refactored to avoid the override: put the code for the regular assert and diagram assert in the same macro, and pass a boolean to the macro to decide which branch to use, this boolean could come from a def in the trait that can be overridden in Diagrams to preserve the current API. This does not allow users to overrides assert themselves like the previous version could, but we can't do that without breaking semantics so that's not a bad thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure. What you propose is a breach of modularity. Assertions now has to know that there is an override in Diagrams. This might work in this concrete example, but I am not sure where else this pattern is used.

So, in the abstract it's better to disallow but in the concrete I personally do not want to deal with the breakage this causes. I'll revert for now and open an issue. If someone wants to take this up, fine.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What you propose is a breach of modularity.

inline defs inherently break modularity, better make that obvious than have surprising semantics IMO. But we can indeed discuss this separately.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking about it, an abstract inline def assert in a base trait would probably suite scalacheck well, but it might require a bigger refactoring.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For scalatest the refactoring seems simple. We can have a single macro in Assertions that takes this as an argument. If this is statically known to be Diagrams we call DiagrammedAssertionsMacro.assert otherwise we call AssertionsMacro.assert. I will try it out.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed it was simple #8601

@odersky odersky force-pushed the change-inline-override branch 2 times, most recently from dad18a2 to 793eaec Compare March 17, 2020 12:56
The previous scheme was more complicated and looks more fragile,
in particular if complex staging operations are applied to a retained
inline method. In that case we have to rely on the fact that the rhs
of a retained inline method is in fact its inlineable body, and not
the retained body.
Copy link
Contributor

@nicolasstucki nicolasstucki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The implementation of the retained methods logic LGTM.

We still need to decide on #8564. I strongly suggest making them final.
Also not sure about the usefulness of the abstract inline.

All these concerns can be handled in followup PRs.

docs/docs/reference/metaprogramming/inline.md Show resolved Hide resolved
@@ -279,6 +279,9 @@ object TastyFormat {

final val INLINEACCESSOR = 21 // The name of an inline accessor `inline$name`

final val BODYRETAINER = 22 // The name of a synthetic method that retains the runtime
// body of an inline method
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is missing from the documentation of Name above (line 40)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants